feat(platform): add file-stream utility #16952
Conversation
This commit introduces a new helper for working with file streams, providing a unified interface for reading local and remote files.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughA new module, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant file-stream
participant fs
participant fetch
participant tmp
Caller->>file-stream: getFileStreamAndMetadata(pathOrUrl)
alt pathOrUrl is local file
file-stream->>fs: stat(pathOrUrl)
file-stream->>fs: createReadStream(pathOrUrl)
file-stream-->>Caller: { stream, metadata }
else pathOrUrl is remote URL
file-stream->>fetch: fetch(pathOrUrl)
alt content-length present
file-stream-->>Caller: { stream, metadata }
else content-length missing
file-stream->>tmp: createTempFile()
file-stream->>fs: write response to temp file
file-stream->>fs: createReadStream(temp file)
file-stream-->>Caller: { stream, metadata }
Note right of file-stream: Temp file cleaned up after stream ends/errors
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
platform/lib/file-stream.tsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
platform/__tests__/file-stream.js (1)
33-33: Consider using dynamic port allocation to avoid conflicts.The hard-coded port 3892 could cause test failures if the port is already in use. Consider using port 0 to let the OS assign an available port.
- const testPort = 3892; + let testPort;Then update the server listen callback to capture the assigned port:
server.listen(0, () => { testPort = server.address().port; resolve(); });platform/lib/file-stream.ts (2)
1-9: Consolidate fs imports for cleaner code.Multiple imports from the
fsmodule can be consolidated into a single import statement.-import { - createReadStream, promises as fs, -} from "fs"; -import { createWriteStream } from "fs"; +import { + createReadStream, createWriteStream, promises as fs, +} from "fs";
82-82: Document the non-null assertion for clarity.The non-null assertion operator
!is used here because we've already checked that the header exists. Consider adding a comment for clarity.- ? new Date(headers.get("last-modified")!) + ? new Date(headers.get("last-modified")!) // Safe: checked for existence above
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
platform/dist/file-stream.d.tsis excluded by!**/dist/**platform/dist/file-stream.jsis excluded by!**/dist/**platform/dist/index.d.tsis excluded by!**/dist/**platform/dist/index.jsis excluded by!**/dist/**platform/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
platform/__tests__/file-stream.js(1 hunks)platform/lib/file-stream.ts(1 hunks)platform/lib/index.ts(3 hunks)platform/package.json(1 hunks)
🔇 Additional comments (5)
platform/package.json (1)
3-3: Version bump is appropriate for new feature addition.The minor version increment from 3.0.3 to 3.1.0 correctly follows semantic versioning for adding new functionality (file-stream utilities).
platform/lib/index.ts (2)
13-19: File-stream exports are properly structured.The new exports for
getFileStreamAndMetadata,getFileStream, andFileMetadatatype are correctly added and follow the existing export pattern.
170-171: ESLint suppressions are reasonable for dynamic types.The
@typescript-eslint/no-explicit-anysuppressions for$eventandbodyare acceptable given their dynamic nature in the platform context.Also applies to: 219-220
platform/__tests__/file-stream.js (1)
1-196: Excellent test coverage for the file-stream module.The test suite comprehensively covers:
- Local and remote file streaming
- Metadata extraction with various HTTP headers
- Error handling for invalid paths/URLs
- Temporary file cleanup scenarios
- Edge cases like missing content-length headers
Great attention to testing resource cleanup!
platform/lib/file-stream.ts (1)
2-2:✅ Verification successful
Verify Node.js version compatibility for stream/web import.
The
stream/webmodule requires Node.js 16.5.0 or later. Ensure your platform supports this version.
🏁 Script executed:
#!/bin/bash # Check if package.json specifies Node.js version requirements fd "package.json" --exec grep -E "engines|node" {} \;Length of output: 2232
No action needed: Node.js ≥18.0.0 already satisfies stream/web requirement
The project’s package.json enforces
"node": ">=18.0.0", which exceeds the 16.5.0 minimum needed for thestream/webimport.
jcortes
left a comment
jcortes
left a comment
There was a problem hiding this comment.
I'm adding a few suggestions with the help of AI!
* Create and use safeStat function * Check for response.body when fetching file * Use uuid package to generate temp file name * Use mime-types package to lookup content type * Cleanup temp file on error Co-authored-by: Jorge Cortes <[email protected]>
* use basename to get file name from file url * lookup content type from file name if no content-type header
|
Thanks for the review @jcortes! All of your suggestions have been applied. |
WHY
Components that take a file as input accept a file path (e.g.,
/tmp/my-file.csv), a remove URL (e.g.https://example.com/file.txt), or both. For convenience, ideally every component that accepts only a file path would also accept a remote file URL. Those components can be updated to use the helper, added in this PR, to get a readable stream and file metadata by passing either a local file path or remote file URL.For example:
Summary by CodeRabbit